Skip to content

JAVA-5949 preserve connection pool on backpressure errors when establishing connections#1900

Open
nhachicha wants to merge 9 commits intomongodb:backpressurefrom
nhachicha:nh/backpressure/preserve_connection_pool
Open

JAVA-5949 preserve connection pool on backpressure errors when establishing connections#1900
nhachicha wants to merge 9 commits intomongodb:backpressurefrom
nhachicha:nh/backpressure/preserve_connection_pool

Conversation

@nhachicha
Copy link
Collaborator

@nhachicha nhachicha commented Feb 25, 2026

Original PR #1854 accidentally closed, and had no outstanding review comments.

This current PR depends on #1856:

  • we should merge the latter into main,
  • then merge main in backpressure,
  • then merge backpressure into the the branch for this PR,
  • and only then can we merge this PR into backpressure.

Specification changes:

JAVA-5949, JAVA-6056, JAVA-6095

@nhachicha nhachicha self-assigned this Feb 25, 2026
@nhachicha nhachicha requested a review from vbabanin February 25, 2026 16:56
@nhachicha nhachicha marked this pull request as ready for review February 25, 2026 16:56
@nhachicha nhachicha requested a review from a team as a code owner February 25, 2026 16:56
@codeowners-service-app
Copy link

Assigned stIncMale for team dbx-java because vbabanin is out of office.

@stIncMale
Copy link
Member

Added the following to the description of the PR (a new piece of work to be done within the PR): DRIVERS-3394: adjust spec test description (#1894)(JAVA-6095).

boolean terminated = executor.awaitTermination(20, SECONDS);
assertTrue("Executor did not terminate within timeout", terminated);

// Assert at least 10 ConnectionCheckOutFailedEvents occurred
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this comment? The assertion message already explains the intent (e.g., “Expected at least 10 ConnectionCheckOutFailedEvents, but got …”).

assertTrue("Expected at least 10 ConnectionCheckOutFailedEvents, but got " + connectionCheckOutFailedEventCount.get(),
connectionCheckOutFailedEventCount.get() >= 10);

// Assert 0 PoolClearedEvents occurred
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +341 to +344
// Teardown: sleep 1 second and reset rate limiter
Thread.sleep(1000);
adminDatabase.runCommand(new Document("setParameter", 1)
.append("ingressConnectionEstablishmentRateLimiterEnabled", false));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cleanup is currently conditional on the code above completing successfully. If an assertion or exception happens earlier, this teardown won’t run, which can leak state into subsequent tests.

We should move it this cleanup into @AfterEach/afterEach so it runs reliably regardless of how the test exits.

Comment on lines +283 to +296
AtomicInteger connectionCheckOutFailedEventCount = new AtomicInteger(0);
AtomicInteger poolClearedEventCount = new AtomicInteger(0);

ConnectionPoolListener connectionPoolListener = new ConnectionPoolListener() {
@Override
public void connectionCheckOutFailed(final ConnectionCheckOutFailedEvent event) {
connectionCheckOutFailedEventCount.incrementAndGet();
}

@Override
public void connectionPoolCleared(final ConnectionPoolClearedEvent event) {
poolClearedEventCount.incrementAndGet();
}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of introducing a new anonymous listener with counters, we can reuse the existing test listener:

TestConnectionPoolListener connectionPoolListener = new TestConnectionPoolListener();

It already provides await helpers that double as assertions and helpers to assert that zero PoolClearedEvents happened, e.g.:

connectionPoolListener.waitForEvent(ConnectionPoolClearedEvent.class, 1, 110, SECONDS);

This keeps the test more concise and reuses established utilities for clarity/consistency.

// clear the pool as they're not related to overload.
// TLS configuration errors (certificate validation, protocol mismatches) should also clear the pool
// as they indicate configuration issues, not server overload.
if (beforeHandshake && !sdamIssue.relatedToAuth() && !sdamIssue.relatedToTlsConfigurationError()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we attach SystemOverloadedError and RetryableError labels in the SDAM error-handling path (effectively only for DefaultServer). In load-balanced mode, SDAM isn’t involved: the LB code path invalidates the pool directly (e.g., connectionPool.invalidate(serviceId, generation)), so the labeling logic is bypassed.

This means users running the driver in LB mode (behind an NLB) can still hit network errors, TLS handshake failures, timeouts during connection establishment or hello, but won’t get the labels.

However, these labels are a CMAP requirement, not SDAM. The CMAP spec states: “The pool MUST add the error labels SystemOverloadedError and RetryableError to network errors or network timeouts it encounters during the connection establishment or the hello message.”

Since this is defined as a pool behavior (topology-agnostic), it seems we should implement the labeling in the connection pool layer so it applies consistently in both default and load-balanced modes.

// clear the pool as they're not related to overload.
// TLS configuration errors (certificate validation, protocol mismatches) should also clear the pool
// as they indicate configuration issues, not server overload.
if (beforeHandshake && !sdamIssue.relatedToAuth() && !sdamIssue.relatedToTlsConfigurationError()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we don’t distinguish DNS lookup failures (UnknownHostException) from other connection-establishment network errors. As a result, a DNS failure goes through the generic path (same as connection reset/timeout) and gets SystemOverloadedError/RetryableError labels.

The CMAP spec excludes DNS failures from backpressure labeling: `“For errors that the driver can distinguish as never occurring due to server overload, such as DNS lookup failures […] the driver MUST NOT add backpressure error labels for these error types.”.

Proposed change: detect DNS failure by walking the exception cause chain for UnknownHostException (it’s wrapped as MongoSocketException from ServerAddressHelper.getSocketAddresses()), and when present, skipping backpressure label attachment so SDAM follows the normal path (clear the pool and mark the server Unknown).

In that case, we should add coverage to assert that labeling and pool clearing behaviour. If the driver ever changes the wrapper exception type MongoSocketException (or stops wrapping UnknownHostException this way) and starts adding labels the test should fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants